-
Notifications
You must be signed in to change notification settings - Fork 7.8k
STM32WB0x: Add PM support (suspend-to-ram) #94402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fix the improper use of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC for some STM32WB0 drivers due to the misunderstanding of its definition. Signed-off-by: Ali Hozhabri <[email protected]>
Provide radio timer driver for STM32WB0x SoCs. Signed-off-by: Ali Hozhabri <[email protected]>
Use radio timer as the system timer when Bluetooth is used. Modify CMakeLists.txt to compile radio timer driver when STM32_RADIO_TIMER is enabled. Remove the common parts from hci_stm32wb0.c that are present in the radio timer driver. Set the appropriate value for SYS_CLOCK_HW_CYCLES_PER_SEC and SYS_CLOCK_TICKS_PER_SEC. Signed-off-by: Ali Hozhabri <[email protected]>
Provide PM support, specifically suspend-to-ram, for STM32WB0x. Enable STM32_RADIO_TIMER Kconfig parameter when PM is set. Signed-off-by: Ali Hozhabri <[email protected]>
Enable UART wake-up line in STM32 driver. Signed-off-by: Ali Hozhabri <[email protected]>
Add PM support to the STM32WB0x Bluetooth HCI driver. Implement PM event register to wake up the device for its BLE events. Signed-off-by: Ali Hozhabri <[email protected]>
Optimize power consumption for the Nucleo-WB09KE board by implementing correct pull-up/pull-down configurations when the device enters lower power states. Signed-off-by: Ali Hozhabri <[email protected]>
config SYS_CLOCK_HW_CYCLES_PER_SEC | ||
default 409600 | ||
depends on STM32_RADIO_TIMER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get this from a dts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a software timer and there is no crystal running at this frequency that's why it is set in Kconfig. What do you think now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardware timer with software configurability? That can be represented in dts, it's going through a hardware block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain more:
SYS_CLOCK_HW_CYCLES_PER_SEC=409600
is the system time unit (STU). One STU is equal to 625/256 μs (about 2.4414 μs). It is independent to the hardware oscillator variation. Every timeout event is expressed in STU. Only before programming the real counter, the time expressed in STU is converted to the hardware timer counting unit internally from the radio timer driver.
Do you still think that it should be defined in a DTS file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so then the radio timer driver is wrong? This value should be the hardware cycles per second, as the name implies, there shouldn't be some software translation in a radio driver that changes this into the actual value that the hardware uses, it should be using the hardware value directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use the LSE (low-speed external 32.768 kHz), the SYS_CLOCK_HW_CYCLES_PER_SEC can be calculated and is well known. However, when using the LSI (low-speed internal), this value varies because the clock frequency is unknown and differs between devices within a certain range. In our Bluetooth protocol implementation, it is easier to use the STU, which is fixed and independent of both LSI and LSE. We manage internally this value according to the different HW configuration.
return 0; | ||
} | ||
|
||
SYS_INIT(board_pupd_init, PRE_KERNEL_2, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
zephyr/include/zephyr/platform/hooks.h
Line 70 in 965bcd6
void board_early_init_hook(void); |
Also, this should be in the SoC hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it be in the SoC hook? Would you please explain more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operations performed appear to not be board specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. These power pull-up/pull-down configurations are board specific.
static struct k_work_delayable hal_radio_timer_work, ble_stack_work; | ||
static struct k_work_delayable ble_stack_work; | ||
|
||
#if CONFIG_PM_DEVICE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if CONFIG_PM_DEVICE | |
#ifdef CONFIG_PM_DEVICE |
apply everywhere for Kconfig symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which style do you suggest? #ifdef
or #if defined()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which style do you suggest?
#ifdef
or#if defined()
?
either is fine.
#elif defined(IS_UART_WAKEUP_FROMSTOP_INSTANCE) | ||
if (wakeup_line != STM32_WAKEUP_LINE_NONE) { | ||
/* Enable EXTI line associated to UART wake-up event */ | ||
LL_EXTI_EnableIT_0_31(BIT(wakeup_line)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the STM32 EXTI API in include/zephyr/drivers/interrupt_controller/intc_exti_stm32.h
* Things that need to be preserved across Deepstop, but | ||
* have no associated driver to backup and restore them. | ||
*/ | ||
#define SRAM DT_CHOSEN(zephyr_sram) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define SRAM DT_CHOSEN(zephyr_sram) | |
#define STM32WB0_SRAM DT_CHOSEN(zephyr_sram) |
|
||
#define RCC_CSR (DT_REG_ADDR(DT_NODELABEL(rcc)) + 0x94) | ||
#define PWR_BASE DT_REG_ADDR(DT_NODELABEL(pwrc)) | ||
#define PWR_SR1 0x10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define PWR_SR1 0x10 | |
#define STM32WB0_PWR_SR1 0x10 |
|
||
#if CONFIG_PM_DEVICE | ||
/* ST Proprietary extended event */ | ||
#define HCI_EXT_EVT 0x82 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define HCI_EXT_EVT 0x82 | |
#define STM32_HCI_EXT_EVT 0x82 |
/* ST Proprietary extended event */ | ||
#define HCI_EXT_EVT 0x82 | ||
#define ACI_HAL_END_OF_RADIO_ACTIVITY_VSEVT_CODE 0x0004 | ||
#define STATE_ALL_BITMASK 0xFFFF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define STATE_ALL_BITMASK 0xFFFF | |
#define STM32_STATE_ALL_BITMASK 0xFFFF |
#define HCI_EXT_EVT 0x82 | ||
#define ACI_HAL_END_OF_RADIO_ACTIVITY_VSEVT_CODE 0x0004 | ||
#define STATE_ALL_BITMASK 0xFFFF | ||
#define STATE_IDLE 0x00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define STATE_IDLE 0x00 | |
#define STM32_STATE_IDLE 0x00 |
|
||
/* Construct net_buf from event data */ | ||
buf = get_rx(buffer_out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the NULL check should be 1st, and it should result in a panic or reset or something if it's not excepted.
Update west.yml to point to the recent changes for hal_stm32. Signed-off-by: Ali Hozhabri <[email protected]>
7e53340
to
be88807
Compare
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
|
Provide PM support, specifically suspend-to-ram.
Add PM support to the Bluetooth HCI driver.
Optimize power consumption for the Nucleo-WB09KE board.
Provide radio timer driver for STM32WB0x SoCs to be used as the system timer when Bluetooth and/or PM are enabled.
Set the appropriate value for SYS_CLOCK_HW_CYCLES_PER_SEC and
SYS_CLOCK_TICKS_PER_SEC when radio timer is used as the system timer.
Enable UART wake-up line in STM32 driver.
Fix the improper use of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC for some
STM32WB0 drivers due to the misunderstanding of its definition.
Update west to point to the recent changes for hal_stm32.
Supplementary PR #303.
@mathieuchopstm, thank you for your support in PM.